Codecov Report

Merging #1038 (cb6ddaf) into main (50144ab) will increase coverage by 7.48%.
The diff coverage is 100.00%.

❗ Current head cb6ddaf differs from pull request most recent head 9c0a06b. Consider uploading reports for the commit 9c0a06b to get more accurate results

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   66.04%   73.53%   +7.48%     
==========================================
  Files         313      365      +52     
  Lines       44908    48240    +3332     
  Branches    19114        0   -19114     
==========================================
+ Hits        29658    35471    +5813     
- Misses      12669    12769     +100     
+ Partials     2581        0    -2581     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50144ab...9c0a06b. Read the comment docs.

While this looks pretty good, I believe that the validation should be implemented strictly within InterfaceReaction::validate() / BlowersMaselInterfaceReaction::validate() as the checks you implemented are specific to those types of reactions. (Edit: I am actually torn on this issue, as within the new framework I am currently implementing, - see Cantera/enhancements#87, - handling things within ReactionRate.cpp would make sense. A lot of this is still under development, so this won't become clear until #995 is merged.)

6 different temperature points from 200K to 10000K

For temperatures, it may make sense to just check the range where species thermo information of reactants and products is valid?

@12Chao Have you been able to update this PR in response to the review? Thanks!

Sorry for the late response, I agree that there could be a better place for validate functions, but I cannot figure out where is more reasonable for these functions given that an Arrhenius class is created before a SurfaceArrhenius class.

You can just evaluate the rate constant at the specified set of temperatures in InterfaceReaction::validate and BlowersMaselInterfaceReaction::validate, right? I think that would alleviate the concern about having a strange Arrhenius::validate method that would likely fail if you ran it for a non-sticking reaction.

You can just evaluate the rate constant at the specified set of temperatures in InterfaceReaction::validate and BlowersMaselInterfaceReaction::validate, right? I think that would alleviate the concern about having a strange Arrhenius::validate method that would likely fail if you ran it for a non-sticking reaction.

Yes, Arrhenius::validate and BlowersMasel::validate functions are only called by InterfaceReaction::validate and BlowersMaselInterfaceReaction::validate for sticking reactions, but I think @ischoegl means that maybe the rate validate function could be placed at some less confused place with the new framework coming up? I agree that adding the validate function into SurfaceArrhenius and BMSurfaceArrhenius classes makes more sense, but I could not get it work because the Arrhenius and BlowersMasel rate classes are created first when adding the surface reactions, maybe this can be achieved with the new reaction framework?

Yes, Arrhenius::validate and BlowersMasel::validate functions are only called by InterfaceReaction::validate and BlowersMaselInterfaceReaction::validate for sticking reactions, but I think @ischoegl means that maybe the rate validate function could be placed at some less confused place with the new framework coming up?

I'm not sure there's that much reason to worry about future changes to use the new ReactionRate classes for interface reactions. If this feature is implemented purely on the InterfaceReaction and BlowersMaselInterfaceReaction classes, it will be easier to make the transition than if this validation is done in the Arrhenius class that doesn't (and shouldn't) know anything about sticking coefficients in the first place.

I agree that adding the validate function into SurfaceArrhenius and BMSurfaceArrhenius classes makes more sense, but I could not get it work because the Arrhenius and BlowersMasel rate classes are created first when adding the surface reactions, maybe this can be achieved with the new reaction framework?

I am not suggesting moving the validation to the SurfaceArrhenius or BMSurfaceArrhenius classes - I'm suggesting that the validation should happen entirely in the corresponding Reaction class's validate method, since this is where the check can be conditional based on the is_sticking_coefficient variable. You already have half of the logic placed there. For example, something like the following pseudo-code:

void InterfaceReaction::validate() {
    ElementaryReaction::validate();
    if (is_sticking_coefficient) {
        for (double T : temperatureList) {
            k = rate.updateRC(log(T), 1.0/T);
            if (k > 1) {
                throw InputFileError("InterfaceReaction::validate", input, "bad sticking factor");
            }
        }
    }
}

I think you'll need to do something a little more complex to validate this for Blowers-Masel reactions, given your parameter space now has two dimensions.

@speth Thanks for the instruction, Could you please give me a hint about how to call the ThermoPhase object in reaction.cpp file so that I can use the delta enthalpy for Blowers-Masel reaction? I failed trying to use m_thermo and thermo_t objects to represent the ThermoPhase .

A Reaction object doesn't inherently have access to a ThermoPhase object to use for this purpose. Even if it did, I think it wouldn't be sufficient to validate using the enthalpy of reaction at the current state of the ThermoPhase (when the reaction is being added) - I think you'd need to consider the enthalpy of reaction at any temperature that's valid for the phase.

Doing that would complicate things a bit -- you'd probably need to pass the Kinetics object as an argument to the Reaction::validate method (from which you can get access to the ThermoPhase object(s), and that would require shuffling some things around to deprecate the current validate method as well.

Thanks for all the help, I have addressed all the comments for this PR and would be appreciated if anyone could review the new changes

@12Chao ... thanks for your work on this. But I do have a philosophical question - checking the sticking coefficient here goes in the direction of a mechanism check, which is historically not really done within Cantera but relegated to external tools (e.g. ChemCheck, see Cantera/enhancements#42). In another context, you'll see some warnings (e.g. thermo data with discontinuities), but that's about that.

Throwing an error means that a (potentially inexperienced) user would have to fix the YAML file in order to proceed. If they just happened to convert a (buggy) CHEMKIN file from some web repository, this would mean that in most cases those users would be stuck and post on the user group, etc.. So the main question here (which goes beyond syntax) would be:

• Would it make sense to just limit the sticking coefficient to 1 by a simple min operation? (Just like rate-of-progress is automatically set to 0 when concentrations become negative).
• Could you run the check you implemented just once at the beginning to trigger the warning? Also, if a user won't ever run at temperatures high enough to be of concern, does there even have to be a warning?

Regarding changing the signature of Reaction::validate() to include Kinetics I do have some reservations ... current checks mostly involve simple syntax/formatting checks, and not more comprehensive kinetic validations. In general, there is a chicken-and-egg issue when defining reactions: you build Kinetics from Reactions, but at the same time need Kinetics to validate a reaction. Keep in mind that there is the possibility of building kinetics from 'scratch' (individual reactions), where a completely coherent approach would be to run the check you're envisioning when a reaction is added to Kinetics (instead of when it is originally created). One step in this direction would be to simply use a different name and call it later, while leaving the original Reaction::validate() as is - as it is really meant for a different kind of 'validation'.

PS: Sorry if this goes beyond my original feedback (which was strictly on code/implementation); I meant to post some update since, but haven't gotten around that until now.

checking the sticking coefficient here goes in the direction of a mechanism check, which is historically not really done within Cantera but relegated to external tools (e.g. ChemCheck, see Cantera/enhancements#42). In another context, you'll see some warnings (e.g. thermo data with discontinuities), but that's about that.

@ischoegl Thanks for the comments. Moving the check to ChemCheck is not a hard task, and I can definately work on that in the future.

• Would it make sense to just limit the sticking coefficient to 1 by a simple min operation? (Just like rate-of-progress is automatically set to 0 when concentrations become negative)

I'm not sure how to deal with the problematic sticking coefficient except raising an error message, but I think at least we need to warn the user if their sticking coefficient exceeds the limit.

Regarding changing the signature of Reaction::validate() to include Kinetics I do have some reservations ... current checks mostly involve simple syntax/formatting checks, and not more comprehensive kinetic validations. In general, there is a chicken-and-egg issue when defining reactions: you build Kinetics from Reactions, but at the same time need Kinetics to validate a reaction. Keep in mind that there is the possibility of building kinetics from 'scratch' (individual reactions), where a completely coherent approach would be to run the check you're envisioning when a reaction is added to Kinetics (instead of when it is originally created). One step in this direction would be to simply use a different name and call it later, while leaving the original Reaction::validate() as is - as it is really meant for a different kind of 'validation'.

I'm a little confused about this comment. The Kinetics input is not used for the validate of other types of reactions but only BlowersMaselInterfaceReactions. Kinetics is used because ThermoPhase is needed in the sticking coefficient calculation for BlowersMaselInterfaceReactions , there is no reaction data involved. Even a new reaction is added, it has to go through the same validation as well, so there should be no problem of validating the new reaction right(because it only needs the Kinetics to obtain species thermo data from the ThermoPhase, sorry if I was wrong)?

• Could you run the check you implemented just once at the beginning to trigger the warning? Also, if a user won't ever run at temperatures high enough to be of concern, does there even have to be a warning?

This is a good point, I am currently just doing the same check like the PLOG reaction validation, but I can try to move the validate functions to KineticsFactory::addReactions and only validate the current temperature point, in that way I don't have to add Kinetics input to all the validate functions either.

I'm a little confused about this comment. [...]

It's a pretty subtle point. A lot of the Reaction derived objects use overloaded methods, so they can be called from the base without having to think about a specific implementation (meaning that all methods are called in the same way). @speth mentioned that the current (argument-less) validate method would have to be deprecated in favor of one that takes Kinetics as an argument to expose thermo data. Extrapolating, this would mean to add Kinetics everywhere even if this is ultimately only needed for a select few cases.

My (admittedly personal) opinion here is to use arguments as frugally as possible (i.e. don't add them unless absolutely needed), which would favor the case against changing the current validate signature. The other subtle point here is that checking things as late as possible gets around the chicken-and-egg problem I previously mentioned. If your check were to be executed when the reaction is added to Kinetics, things flow naturally. As things stand now, Reaction::validate is called when the object is instantiated. All of this is admittedly subtle - so take it with a grain of salt.

Thanks for the suggestion! I'll try to come up with a better way to avoid the changes to these validate functions.

Thanks for the suggestion! I'll try to come up with a better way to avoid the changes to these validate functions.

Sure. But wait for others to weigh in also - whatever you do, these subtle points shouldn't keep you from wrapping this work up. Passing Kinetics isn't a problem with the code as it stands, and my preference to run some of those checks as late as possible may not be universally shared. I am just hoping to convince everyone eventually ...

My main point is that you should be careful raising an error where other codes (chemkin?) may not find an issue. Having a converted buggy mechanism fail would create some unnecessary repercussions on the user group where simply limiting the output to 1 would be easier. Just looking at the code, having k = std::min(1., updateRC(logT, recipT)); (if I understand how this limit is applied correctly) would sidestep things for the most part (this could even be checked within updateRC itself). Issuing a warning would certainly be a benefit of course.

My (admittedly personal) opinion here is to use arguments as frugally as possible (i.e. don't add them unless absolutely needed), which would favor the case against changing the current validate signature.

I agree in general, but I think that this isn't actually the only case where having the Kinetics (and by extension, ThermoPhase) object present for validating a reaction would be helpful. For instance, it could also be used to select the temperatures used for validating P-log rates based on the valid temperature range for the phase.

If your check were to be executed when the reaction is added to Kinetics, things flow naturally. As things stand now, Reaction::validate is called when the object is instantiated. All of this is admittedly subtle - so take it with a grain of salt.

The point where validation is called isn't changed here, is it? Generally, it will be called at the point when a reaction is added to the Kinetics object. The only case where it happens when a Reaction is first created is when the getReactions function is called. Whether that makes sense or not seems orthogonal to this PR.

But I do have a philosophical question - checking the sticking coefficient here goes in the direction of a mechanism check, which is historically not really done within Cantera but relegated to external tools (e.g. ChemCheck, see Cantera/enhancements#42). In another context, you'll see some warnings (e.g. thermo data with discontinuities), but that's about that.

Throwing an error means that a (potentially inexperienced) user would have to fix the YAML file in order to proceed. If they just happened to convert a (buggy) CHEMKIN file from some web repository, this would mean that in most cases those users would be stuck and post on the user group, etc..

I think it's possible to make a useful distinction between checking for issues in an input file and a tool for helping figure out what's actually wrong with the input file and how to fix it. I'd say the first part is something that is good to build directly into Cantera. Otherwise, people are going to just blithely use mechanisms with these problems and are not going to do the due diligence of running them through ChemCheck.

I do agree that automatically treating this as an error might be a bit extreme, and by analogy with how Cantera treats discontinuous thermo data, it might make sense to have the default behavior here be to just issue a warning. I'm sure we'll still get some posts to the Users' Group, though.

I think that this isn't actually the only case where having theKinetics (and by extension, ThermoPhase) object present for validating a reaction would be helpful.

and

I think it's possible to make a useful distinction between checking for issues in an input file and a tool for helping figure out what's actually wrong with the input file and how to fix it.

👍 I agree ...

The point where validation is called isn't changed here, is it? Generally, it will be called at the point when a reaction is added to the Kinetics object.

This is technically true for the YAML route, but only as things happen in a direct sequence. As you mentioned, this is not universally true: validate is typically called from a Reaction constructor, which doesn't necessarily require an immediate addition to Kinetics for some other creation routes.

My main point is that I hope to avoid an across-the-board void validate(const Kinetics& kin) signature ... I'd much prefer to have one set of checks that do not rely on Kinetics (i.e. these would trigger failures at the Reaction level), and a second set of checks that require more in-depth information from Kinetics (which would fail when those reactions are added to Kinetics). Much of this can be differentiated in separate PR's, as this goes well beyond of what was originally proposed here. Having separate void validate() and void validate(const Kinetics& kin) is all I am asking for at the moment (it would help to have a more meaningful nomenclature eventually).

In other words, I am asking for

//! @deprecated To be removed after Cantera 2.6
virtual void validate();

to be reconsidered.

I can keep the validation function as is, if we all agree on throw a warning instead of an error.

Another way to raise the warning for problematic sticking coefficient at the current temperature is to add

    for (size_t n = 0; n < m_stickingData.size(); n++) {
        const StickData& item = m_stickingData[n];
        if (kf[item.index] > 1) {
            double T = thermo(surfacePhaseIndex()).temperature();
            warn_user("InterfaceKinetics::applyStickingCorrection",
                      "\nSticking coefficient of reaction {} is bigger than 1 at {} K.",
                      m_reactions[item.index]->equation(), T);
        }
        if (item.use_motz_wise) {
            kf[item.index] /= 1 - 0.5 * kf[item.index];
        }
        kf[item.index] *= factors[n] * sqrt(T) * item.multiplier;

in InterfaceKinetics::applyStickingCorrection, it will check if a forward rate constant is more than 1 before applying the correction. Since InterfaceKinetics::applyStickingCorrection is called in InterfaceKinetics::_update_rates_T, so it will throw a warning if there is a problem with sticking coefficients every time the forward_rate_constants function is called. This way the code modification is the least, but it may affect the code efficiency as it does extra checks when forward_rate_constants is called. Any suggestion on which way I should go for would be appreciated! @rwest @bryanwweber @speth @ischoegl

Having separate void validate() and void validate(const Kinetics& kin) is all I am asking for at the moment (it would help to have a more meaningful nomenclature eventually).

OK, that makes sense to me. Being able to catch issues that don't need the Kinetics object when the Reaction is first instantiated would have some benefits.

@12Chao, can you update to reflect this consensus? I hope you still have a branch with the version where you added the void validate(const Kinetics& kin) function?

I have updated the code, but I didn't figure out how to write a test to catch the CanteraWarning . I tried to catch is as a CanteraError, but it didn't work. Is there another way that I could try?

@12Chao ... The way I've seen this done is to make Cantera warnings fatal (Cantera::make_deprecation_warnings_fatal(); in C++, or ct.make_deprecation_warnings_fatal() in Python). If this flag is set, warnings are converted to errors and you can test for correct behavior.

Caveat: This is for deprecation errors and thus is not necessarily the easiest approach ... I guess I responded too fast.

Yeah, I don't think we currently have a way of testing warnings issued by the C++ warn_user command.

If you want to be able to test the warning message being generated here, I guess you could add a similar global flag like we use for deprecation warnings to temporarily make the warnings into errors and catch that exception -- see the implementation of Application::warn_deprecated and the functions related to the Application::m_fatal_deprecation_warnings variable as a kind of template.

This is related to Cantera/enhancements#112, where the feature described there would make it easier to make trap the warning in a Python test.

One short-term option is to just have a test that issues this warning and leave testing whether that is actually the case as a "TODO" item, and we can accept that the fact that the test coverage will show this bit of code being called as good enough for now.

Thanks! I have made a new function to convert CanteraWarning into CanteraError and used in the test.

Thanks, @12Chao. To fix some merge conflicts that arose as a result of #1101 (I think), I ended up having to squash most of the commits here down into one.